feat(xpkg): add --annotation flag to xpkg build and xpkg push#11
feat(xpkg): add --annotation flag to xpkg build and xpkg push#11chaitanyapantheor wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughAdds repeatable ChangesOCI Annotation CLI Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Chaitanya Maili <[email protected]>
Signed-off-by: Chaitanya Maili <[email protected]>
Signed-off-by: Chaitanya Maili <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]> Signed-off-by: Chaitanya Maili <[email protected]>
4476efa to
a8a515b
Compare
adamwg
left a comment
There was a problem hiding this comment.
I've left a couple of thoughts, but overall this looks good to me and seems like a valuable feature.
| Package string `arg:"" help:"Where to push the package. Must be a fully qualified OCI tag, including the registry, repository, and tag." placeholder:"REGISTRY/REPOSITORY:TAG"` | ||
|
|
||
| // Flags. Keep sorted alphabetically. | ||
| Annotation []string `help:"An OCI manifest annotation to add to the package in key=value format. Repeatable." placeholder:"KEY=VALUE" short:"a"` |
There was a problem hiding this comment.
I wonder if this should be OCIAnnotation / --oci-annotation or something like that to make it clear how it's different from the metadata.annotations.
There was a problem hiding this comment.
Good point — renamed to --oci-annotation (Go field OCIAnnotation, explicit name:"oci-annotation" kong tag) in both xpkg build and xpkg push. Short flag -a is unchanged.
| return errors.Wrapf(err, errAnnotateLayers) | ||
| } | ||
|
|
||
| img = annotateImage(img, annotations) |
There was a problem hiding this comment.
Should we apply the same annotations to the index (built below with mutate.AppendManifests)?
There was a problem hiding this comment.
Added annotateIndex helper (mirrors annotateImage) and applied it to the index before remote.WriteIndex in the multi-platform path. Annotations now appear on both the individual manifests and the top-level index.
| @@ -0,0 +1,49 @@ | |||
| /* | |||
| Copyright 2025 The Crossplane Authors. | |||
There was a problem hiding this comment.
Fixed — updated to 2026.
- Rename --annotation to --oci-annotation in xpkg build and push to distinguish from Kubernetes metadata.annotations - Apply OCI annotations to the image index in the multi-platform push path, not only to individual manifests - Fix copyright year in annotations.go and annotations_test.go Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/crossplane/xpkg/annotations.go (1)
30-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider validating that annotation keys are non-empty.
The current implementation would accept an annotation like
=value(empty key), which could cause confusing errors later when pushing to the registry. Adding a simple validation would provide users with a clearer, earlier error message.🛡️ Suggested validation
func parseAnnotations(kvs []string) (map[string]string, error) { anns := make(map[string]string, len(kvs)) for _, kv := range kvs { k, v, ok := strings.Cut(kv, "=") if !ok { return nil, errors.Errorf("invalid annotation %q: must be in key=value format", kv) } + if k == "" { + return nil, errors.Errorf("invalid annotation %q: key cannot be empty", kv) + } anns[k] = v } return anns, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/xpkg/annotations.go` around lines 30 - 40, The parseAnnotations function accepts "k=v" pairs but doesn't reject empty keys (e.g., "=value"); update parseAnnotations to validate that the extracted key (variable k from strings.Cut on each kv) is non-empty and, if empty, return a clear formatted error (similar to the existing errors.Errorf) indicating the annotation has an empty key; ensure this check happens after the strings.Cut ok check and before assigning into anns so no empty-key entries are inserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/crossplane/xpkg/annotations.go`:
- Around line 30-40: The parseAnnotations function accepts "k=v" pairs but
doesn't reject empty keys (e.g., "=value"); update parseAnnotations to validate
that the extracted key (variable k from strings.Cut on each kv) is non-empty
and, if empty, return a clear formatted error (similar to the existing
errors.Errorf) indicating the annotation has an empty key; ensure this check
happens after the strings.Cut ok check and before assigning into anns so no
empty-key entries are inserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0477f7aa-6ce3-4b3c-953a-d9cd84c3a305
📒 Files selected for processing (4)
cmd/crossplane/xpkg/annotations.gocmd/crossplane/xpkg/annotations_test.gocmd/crossplane/xpkg/build.gocmd/crossplane/xpkg/push.go
✅ Files skipped from review due to trivial changes (1)
- cmd/crossplane/xpkg/annotations_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/crossplane/xpkg/build.go
- cmd/crossplane/xpkg/push.go
Description of your changes
Adds a repeatable
--annotation/-a KEY=VALUEflag tocrossplane xpkg buildand
crossplane xpkg push, allowing users to attach OCI manifest annotationsto packages at build or push time.
Annotations are applied to the OCI image manifest. Malformed annotations
(missing =) return an error before any write or push occurs. Reading
annotations from crossplane.yaml is intentionally out of scope to avoid
silently propagating internal Crossplane annotations to the OCI registry.
Covered by unit tests in cmd/crossplane/xpkg/annotations_test.go.
Fixes crossplane/crossplane#7282
I have:
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.